-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Demo fixes #136
Demo fixes #136
Conversation
…to measure-index_85
…ure into measure-index_85
…to measure-index_85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments/questions inline; approving in case you want to merge this in and fix stuff up later.
gatsby-node.js
Outdated
return await response.json() | ||
return DUMMY_DATA[endpoint] | ||
|
||
// return await response.json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep using real data for the other endpoints - mind switching this to only use fake data for referendums?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; I was getting errors about undefined not having fields.slug but that seemed to fix it. If you aren't having that issue I might have made a mistake setting up my back end somewhere then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's probably because of the elections[0]
below. If you're going to make that change in another PR then you can just re-enable response.json()
there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok nevermind, I'm not sure what's causing that error, that should be fixed with #135.
gatsby-node.js
Outdated
referendum.Title.toLowerCase() | ||
.split(" ") | ||
.join("-"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you configured the slug plugin for this node, can we use fields.slug instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought I got all of them! Guess not!
gatsby-node.js
Outdated
node.Referendums.forEach(referendum => { | ||
createPage({ | ||
path: | ||
"/" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A template string might be a little easier to read here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in new commit
type MeasuresJson implements Node { | ||
electionDate: String! | ||
title: String! | ||
description: String! | ||
ballotLanguage: String! | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to delete the JSON files entirely? That's fine if we're going to get all the data we need from the API, but I was under the impression we'd need to have some client-side data for the measure description, etc. If we no longer need that, let's also delete the json files under /src/data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that was for the JSON; I'll put it back in.
type Referendum implements Node { | ||
Title: String! | ||
Description: String | ||
Total_Contributions: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what the API returns? Can we ask to remove the underscore for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the API returns a "Total Contributions"
key, I wasn't sure how to put that in since it threw an error when I used quotes, and I think it threw an error when I put it in two words too? I couldn't find how to define a string like that but I saw the _ being used elsewhere and it worked on the Referendums, so I left it in. Maybe we should get @geleazar1000111 to change the field to TotalContributions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds like we should get rid of the space on the backend if we can
const [menu, menuOptions] = formatMenu(OfficeElections) | ||
const sections = Object.keys(menu) | ||
const { Date, OfficeElections, Referendums } = | ||
data.allElection?.edges?.[0]?.node ?? [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the recent API changes you'll need to access elections like this:
data.allElections.edges['11/3/2020'].node
(I think I should've updated the dummy data as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I'll make a new PR with all of those changes, since i'll have to jump in a ton of files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, wait. This should be breaking everywhere else if that's the case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh yeah nevermind, this should be fine :)
Fixes from the demo with Diane; please merge only after merging in changes from
measure-index_85
#137